Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ncu-config: introduce project config file #364

Merged
merged 1 commit into from
Oct 4, 2019
Merged

ncu-config: introduce project config file #364

merged 1 commit into from
Oct 4, 2019

Conversation

mmarchini
Copy link
Contributor

Project config file is intended to be committed to a project. This will
allow other projects in the org to use NCU without users having to set
owner and repo by themselves. In the future we can also use this add
other project-specific configurations (number of hours to wait, minimum
approvals and main CI, for example).

@mmarchini
ncu-config: introduce project config file
c956242 
Project config file is intended to be committed to a project. This will
allow other projects in the org to use NCU without users having to set
`owner` and `repo` by themselves. In the future we can also use this add
other project-specific configurations (number of hours to wait, minimum
approvals and main CI, for example).
@codecov
Copy link

codecov bot commented Oct 1, 2019 •
edited
Loading

Codecov Report

Merging #364 into master will increase coverage by 0.1%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #364     +/-   ##
=========================================
+ Coverage   75.08%   75.19%   +0.1%     
=========================================
  Files          21       21             
  Lines        1397     1407     +10     
=========================================
+ Hits         1049     1058      +9     
- Misses        348      349      +1
Impacted Files Coverage Δ
lib/config.js 80.95% <84.61%> (+2.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72fc10a...c956242. Read the comment docs.

@mmarchini mmarchini mentioned this pull request Oct 1, 2019
Closed
@priyank-p
Copy link
Contributor

Shouldn't we just rather look for ~/.ncurc by default without having to pass in -g or --global flag? and use that if it exists. And not look for it if -p is passed and rather use ./.ncurc.

(I don't use the tool so can't tell for sure that this is indeed better implementation; this is just a idea from looking at the code)

@mmarchini
Copy link
Contributor Author

Today ncu-config will look at $PWD/.ncu/config (known as local configs in the source code) if no flags are passed, I didn't want to change this behavior. Also, ncu-config is used only for config manipulation, so it makes sense for the user to explicitly say which config file they want to change/look. Maybe it would make sense for ncu-config list to show configs from all files (like git config does), but I don't think many users use list to make a difference.

All other tools rely on getMergedConfigs, which will fetch configs from all three locations and merge them, prioritizing local config, then project config, then global config.

@joyeecheung
Copy link
Member

Why do we need arguments for these? I think we can simply let ./.ncu/config override ./.ncurc which overrides ~/.ncurc?

@mmarchini
Copy link
Contributor Author

Why do we need arguments for these?

The arguments are only for ncu-config, which is how it worked before (--global to modify ~/.ncurc, no arguments to modify ./.ncu/config). Without the arguments we wouldn't be able to set values (ncu-config set) for a specific file.

I think we can simply let ./.ncu/config override ./.ncurc which overrides ~/.ncurc?

That's the behavior for git-node and other commands, no extra arguments needed.

@joyeecheung joyeecheung merged commit eb5c8c7 into nodejs:master Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mmarchini @priyank-p @joyeecheung